Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(analyzer): Add a test for dangling embed directives / GoMod #7647

Merged
merged 2 commits into from
Oct 6, 2023
Merged

fix(analyzer): Add a test for dangling embed directives / GoMod #7647

merged 2 commits into from
Oct 6, 2023

Conversation

fviernau
Copy link
Member

@fviernau fviernau commented Oct 6, 2023

The GoMod integration does not execute go generate/go build, so the embed directive in main.go points to a non-existing file. When GoMod executes [1] it failed [2] with the previously used Go version 1.20.5. As of [3], with Go version 1.21.1, that issue is fixed.

Add a test to ensure it keeps on working.

Note: It is essential to pass json=Module instead of not just
-json [4], because otherwise the issue would still persist on
Go 1.21.1.

Fixes #5560.

[1] go list -deps -json=Module -buildvcs=false ./...
[2] pattern file.txt: no matching
[3] 47e4520
[4] 43ad3a7

Please ensure that your pull request adheres to our contribution guidelines. Thank you!

@fviernau fviernau requested a review from a team as a code owner October 6, 2023 06:56
@fviernau fviernau force-pushed the gomod-dangling-embed-directive branch 3 times, most recently from 563e9f0 to aba9c4d Compare October 6, 2023 07:01
@fviernau fviernau changed the title test(analyzer): Add a test for dangling embed directives / GoMod fix(analyzer): Add a test for dangling embed directives / GoMod Oct 6, 2023
sschuberth
sschuberth requested changes Oct 6, 2023
View reviewed changes
analyzer/src/main/kotlin/managers/GoMod.kt
@@ -100,7 +100,7 @@ class GoMod(

override fun transformVersion(output: String) = output.removePrefix("go version go").substringBefore(' ')

override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.19.0")
override fun getVersionRequirement(): RangesList = RangesListFactory.create(">=1.21.1")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message:

  • "this projects" -> "the projects"

analyzer/src/funTest/assets/projects/synthetic/gomod-dangling-embed/go.mod
@@ -0,0 +1,3 @@
module gomod_dangling_embed

go 1.19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be 1.21.1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why exactly should it be? (As-is it is consistent with the other test cases in GoModFunTest).

One aspect of the bug fix is in the tooling of version 1.21, but that tooling can be used with 1.19 projects, so I do not see an issue here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One aspect of the bug fix is in the tooling of version 1.21, but that tooling can be used with 1.19 projects

That was the missing piece of information for me here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was the missing piece of information for me here.

Ok, so are we good to go?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that regard yes, but I does not make sense for me to approve before the commit message is fixed.

@codecov
Copy link

codecov bot commented Oct 6, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ff9d65a) 68.03% compared to head (a295ca4) 68.03%.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main    #7647   +/-   ##
=========================================
  Coverage     68.03%   68.03%           
  Complexity     2023     2023           
=========================================
  Files           344      344           
  Lines         16727    16727           
  Branches       2372     2372           
=========================================
  Hits          11381    11381           
  Misses         4363     4363           
  Partials        983      983
Flag Coverage Δ
funTest-non-docker 36.45% <ø> (ø)
test 35.54% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
analyzer/src/main/kotlin/managers/GoMod.kt 79.59% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fviernau fviernau force-pushed the gomod-dangling-embed-directive branch from aba9c4d to 8532c14 Compare October 6, 2023 07:55
@fviernau fviernau requested a review from sschuberth October 6, 2023 07:55
@fviernau fviernau force-pushed the gomod-dangling-embed-directive branch from 8532c14 to 97b10c6 Compare October 6, 2023 07:56
@sschuberth
Copy link
Member

Note that commit-lint still fails.

@fviernau fviernau force-pushed the gomod-dangling-embed-directive branch from 97b10c6 to 14c3077 Compare October 6, 2023 08:13
@fviernau
Copy link
Member Author

fviernau commented Oct 6, 2023 •
edited
Loading

Note that commit-lint still fails.

It seems it flags references to issues using hash in the middle of a sentence without a preceding keyword like "Fixes".
Do you know by any chance an alternative way of referencing the issue compatible with commit lint ?

My work around is to use a reference e.g. [5].

@fviernau fviernau enabled auto-merge (rebase) October 6, 2023 08:17
@sschuberth
Copy link
Member

My work around is to use a reference e.g. [5].

That's as well the only work-around I'm aware of.

@fviernau fviernau disabled auto-merge October 6, 2023 10:16
@fviernau
fix(analyzer): Add a test for dangling embed directives / GoMod
b596b49 
The `GoMod` integration does not execute `go generate/go build`, so the
embed directive in `main.go` points to a non-existing file. When `GoMod`
executes [1] it failed [2] with the previously used Go version 1.20.5.
As of [3], with Go version 1.21.1, that issue is fixed.

Add a test to ensure it keeps on working.

Note: It is essential to pass `json=Module` instead of not just
      `-json` [4], because otherwise the issue would still persist on
      Go 1.21.1. So, [3] and [4] together fix [5].

Fixes #5560.

[1] `go list -deps -json=Module -buildvcs=false ./...`
[2] `pattern file.txt: no matching`
[3] 47e4520
[4] 43ad3a7
[5] #5560.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau
chore(GoMod): Raise the version requirement
a295ca4 
Ensure that projects containing the embed directive can be analyzed
without issues, see also the preceeding commit.

Signed-off-by: Frank Viernau <[email protected]>
@fviernau fviernau force-pushed the gomod-dangling-embed-directive branch from 14c3077 to a295ca4 Compare October 6, 2023 10:16
@fviernau fviernau enabled auto-merge (rebase) October 6, 2023 10:16
@fviernau fviernau merged commit 89c626e into main Oct 6, 2023
21 of 22 checks passed
@fviernau fviernau deleted the gomod-dangling-embed-directive branch October 6, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sschuberth sschuberth sschuberth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GoMod: go list -deps error: no matching files found
2 participants
@fviernau @sschuberth